[CONFIGURATION] Apply general attribute_limits to tracer and logger providers - #4468
[CONFIGURATION] Apply general attribute_limits to tracer and logger providers#4468ayush-singh-0601 wants to merge 9 commits into
Conversation
…roviders The YAML parser already accepted attribute_limits, but SdkBuilder ignored them. Pass the general limits into CreateTracerProvider and CreateLoggerProvider, and use them when the provider has no model-specific limits. Provider limits still take precedence. Fixes open-telemetry#4467
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4468 +/- ##
==========================================
+ Coverage 83.09% 83.15% +0.06%
==========================================
Files 519 520 +1
Lines 20253 20317 +64
==========================================
+ Hits 16827 16892 +65
+ Misses 3426 3425 -1
🚀 New features to boost your workflow:
|
|
can you please any changes that i need to make or is it fine |
|
all checks are passed , please tell if any more changes needed |
dbarker
left a comment
dbarker
left a comment
There was a problem hiding this comment.
Thanks for the fix! Requesting a minor change noted below.
|
@dbarker done |
dbarker
left a comment
dbarker
left a comment
There was a problem hiding this comment.
Thanks for the updates. Diving deeper into the spec and use cases I'm requested a change to the limit models and application methods to meet the spec.
Make each limit on AttributeLimits, SpanLimits, and LogRecordLimits optional so omitted keys and YAML null are unset. Merge uses the model-specific value if set, else the general attribute_limits value, else the model-specific default.
|
@dbarker updated. Limit fields are optional now so omitted/null is distinct from a set value. Merge is model-specific if set, else general attribute_limits, else the model default. |
|
Thanks for syncing main into the branch. I checked the latest run, and all eight GitHub Actions workflows are currently marked action_required, so they have not actually executed yet. Could you approve them when convenient? The previous run did show IWYU, formatting, and several C++14/CMake failures, so once the fresh run is available I will address any remaining PR-specific issues together. Thanks! |
|
I pushed c995d4e with the CI fixes: applied clang-format 18, added the direct optional_value.h includes requested by IWYU, and removed the C++14 ODR-use that caused the sdk_builder_test linker failures. The local format check, diff check, and C++14 smoke compile pass. All eight replacement workflows are showing action_required again, so could you approve this run when convenient? Thanks! |
|
Both latest review comments are addressed in 9643298, with focused regression tests. The eight replacement workflows are currently marked action_required; could you approve this run when convenient? Thanks! |
|
All checks passed if any more changes required , please let me know |
2 participants
Fixes #4467
The YAML parser already accepted
attribute_limits, butSdkBuilderlogged a warning and ignored them. Per the spec, general attribute limits should apply when tracer/loggerlimitsare not set, and model-specific limits should win when they are.Changes
attribute_limitsfromCreateConfiguredSdkintoCreateTracerProviderandCreateLoggerProvider.limits, copyattribute_count_limitandattribute_value_length_limitfrom the general config intoSpanLimits/LogRecordLimits.limits, keep using those values (including span-only fields such as event/link limits).CreateTracerProviderandCreateLoggerProvidertake an optionalAttributeLimitsConfiguration*that defaults tonullptr.CreateConfiguredSdkwiring for both traces and logs.How changes were tested
SdkBuilder.SpanLimitsFromAttributeLimitsandSdkBuilder.SpanLimitsOverrideAttributeLimits.GetSpanLimits()on the configured tracer provider.I could not run the C++ test binaries in this environment (no local CMake/Bazel build of opentelemetry-cpp). CI should run
sdk_builder_testandprogrammatic_configuration_test.CHANGELOG.mdupdated for non-trivial changes